Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add fallible version of the digital traits and deprecate the current ones #108

Merged
merged 3 commits into from
Dec 11, 2018
Merged

Add fallible version of the digital traits and deprecate the current ones #108

merged 3 commits into from
Dec 11, 2018

Conversation

eldruin
Copy link
Member

@eldruin eldruin commented Oct 28, 2018 •
edited
Loading

There seems to be an agreement in #100 on how to proceed with the fallible traits.
This adds the fallible traits under digital::v2 and marks the current ones as deprecated.
This solves #95

@eldruin eldruin requested review from ilya-epifanov, thejpster and a team as code owners October 28, 2018 07:45
@eldruin eldruin mentioned this pull request Oct 28, 2018
Closed
therealprof
therealprof previously approved these changes Oct 28, 2018
View reviewed changes
Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks very much for keeping at it.

@therealprof
Copy link
Contributor

@hannobraun @ryankurte Are you okay with this?

@therealprof
Copy link
Contributor

@rust-embedded/hal Any other opinions?

hannobraun
hannobraun reviewed Oct 28, 2018
View reviewed changes
Copy link
Member

@hannobraun hannobraun left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Sorry, accidentally submitted this comment early. I edited it to complete it.)

It's possible to add additional information to #[deprecated], namely the version since when something has been deprecated, and a note that is going to be part of the compiler warning (I think). This is documented here: https://doc.rust-lang.org/reference/attributes.html

It would be nice to have this, but I'm fine merging this pull request without.

hannobraun
hannobraun previously approved these changes Oct 28, 2018
View reviewed changes
Copy link
Member

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant to approve this pull request, but accidentally submitted my comment before it was complete. See my other comment for a note on a nice-to-have item.

Thank you @eldruin for sticking with this! I know working on this project is not easy :)

@eldruin eldruin dismissed stale reviews from hannobraun and therealprof via 421d11b October 28, 2018 15:30
hannobraun
hannobraun previously approved these changes Oct 28, 2018
View reviewed changes
Copy link
Member

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again, @eldruin! You're really going above and beyond what can reasonably be expected. I really appreciate it!

ryankurte
ryankurte previously approved these changes Oct 28, 2018
View reviewed changes
Copy link
Contributor

@ryankurte ryankurte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Thanks again @eldruin for working with us through all this.

We don't currently have an adaptor between v1 and v2, do we want to provide that? (I suggest as a separate PR if so).

@ryankurte ryankurte mentioned this pull request Nov 3, 2018
Closed
@caemor
Copy link

caemor commented Nov 3, 2018

Is there anything missing here?

@ryankurte
Copy link
Contributor

We still need adapter traits (see #100 for examples) before we release, but I'll merge this for now.

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 13, 2018

👎 Rejected by label

@ryankurte
Copy link
Contributor

Oops, @therealprof are you still happy with this?

@therealprof
Copy link
Contributor

@ryankurte Yepp, I'm good. Needs conflict resolution, though.

therealprof
therealprof previously approved these changes Nov 13, 2018
View reviewed changes
@ryankurte ryankurte dismissed stale reviews from therealprof, hannobraun, and themself via 14cac99 November 28, 2018 00:35
@ryankurte
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 28, 2018

👎 Rejected by too few approved reviews

@ryankurte
Copy link
Contributor

whoops

bors r+

bors bot added a commit that referenced this pull request Nov 28, 2018
@bors @eldruin @ryankurte
Merge #108
63a553c 
108: Add fallible version of the digital traits and deprecate the current ones r=ryankurte a=eldruin

There seems to be an agreement in [#100](#100 (comment)) on how to proceed with the fallible traits.
This adds the fallible traits under `digital::v2` and marks the current ones as deprecated.

Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
Co-authored-by: Ryan <ryankurte@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Nov 28, 2018

Build failed

@therealprof
Copy link
Contributor

Funny complaints from the nightly compiler. 👎

@rust-embedded/resources Should we make the nightly compiler builds informal only, i.e. not fail CI? Our main target is beta/stable anyways.

@caemor
Copy link

caemor commented Nov 28, 2018

cargo test is only run on nightly atm.

This might be because two of the doc tests rely on nightly features (https://github.com/rust-embedded/embedded-hal/blob/master/src/lib.rs#L385 ++ and https://github.com/rust-embedded/embedded-hal/blob/master/src/lib.rs#L557 ++). Maybe the doc test for these two should be tagged with the ignore setting until these features are stable/beta.

but either way the failed doc test doesn't have anything to do with this PR.

@ryankurte
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Dec 11, 2018
@bors
Try #108:
77adbac
@bors
Copy link
Contributor

bors bot commented Dec 11, 2018

try

Build succeeded

@ryankurte
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Dec 11, 2018
@bors @eldruin @ryankurte
Merge #108
d8c78cc 
108: Add fallible version of the digital traits and deprecate the current ones r=ryankurte a=eldruin

There seems to be an agreement in [#100](#100 (comment)) on how to proceed with the fallible traits.
This adds the fallible traits under `digital::v2` and marks the current ones as deprecated.

Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
Co-authored-by: Ryan <ryankurte@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Dec 11, 2018

Build succeeded

@bors bors bot merged commit 14cac99 into rust-embedded:master Dec 11, 2018
@eldruin eldruin deleted the add-fallible-digital-traits branch December 12, 2018 07:30
@eldruin eldruin mentioned this pull request Dec 12, 2018
Closed
@eldruin eldruin mentioned this pull request Jun 8, 2019
Closed
@thejpster thejpster mentioned this pull request Nov 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryankurte ryankurte ryankurte approved these changes

@hannobraun hannobraun hannobraun left review comments

@therealprof therealprof therealprof left review comments

@ilya-epifanov ilya-epifanov Awaiting requested review from ilya-epifanov

@thejpster thejpster Awaiting requested review from thejpster

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eldruin @therealprof @caemor @ryankurte @hannobraun